Conversation
🦋 Changeset detectedLatest commit: b68b8c0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Fixes the issue with field unmount in core.
|
View your CI Pipeline Execution ↗ for commit b68b8c0
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2068 +/- ##
==========================================
- Coverage 90.35% 90.24% -0.11%
==========================================
Files 38 49 +11
Lines 1752 2041 +289
Branches 444 533 +89
==========================================
+ Hits 1583 1842 +259
- Misses 149 179 +30
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Form option Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Form as FormApi
participant Field as FieldApi
participant Val as ValidationSystem
App->>Form: create(formOptions with cleanupFieldsOnUnmount=true)
App->>Field: mount field
Field->>Form: register field instance & metadata
Field->>Val: start validations / set timeouts / listeners
Field-->>App: return cleanup function
Note over Val: validations/timeouts/listeners in-flight
App->>Field: call cleanup() on unmount
Field->>Val: abort controllers & cancel timeouts/listeners
Val-->>Val: stop async callbacks / ignore late results
Field->>Form: reset field meta (or preserve depending on option), clear instance reference
Field-->>App: cleanup complete
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1361-1375: The abort loop currently runs before checking whether
this FieldApi instance is still the active one, which can cancel validations for
a newer remounted instance; move the instance check so it runs before touching
shared state (i.e., check fieldInfo.instance !== this and return early), or
alternatively inside the loop verify fieldInfo.instance === this before calling
validationMeta?.lastAbortController.abort() and clearing entries; update the
code paths around fieldInfo, validationMetaMap, lastAbortController, and the
instance guard to ensure only the active FieldApi instance aborts and clears the
shared validationMetaMap.
- Around line 1377-1383: The current reset in FieldApi using
this.form.baseStore.setState(...) replaces prev.fieldMetaBase[this.name] with
defaultFieldMeta and thus wipes preserved state on unmount/remount; update the
setter to merge with any existing meta for this.name instead of overwriting so
preserved flags/values survive (e.g., read existing =
prev.fieldMetaBase?.[this.name], then set fieldMetaBase[this.name] = {
...defaultFieldMeta, ...existing } or selectively preserve keys like
touched/value/defaultValueSeeded) so the logic in FieldApi that reseeds
options.defaultValue (referenced by the FieldApi methods around lines that
handle defaultValue reseeding) no longer loses user-entered values on remount.
In `@packages/react-form/tests/useField.test.tsx`:
- Around line 422-423: The test asserts onSubmit immediately after clicking
submit but form.handleSubmit() executes asynchronously; update the test to wait
for the async submit path to complete before asserting by awaiting a wait helper
that checks onSubmit (e.g., use waitFor or another async wait) so the assertion
verifies that onSubmit was called after form._handleSubmit()/form.handleSubmit()
finishes; reference the submitButton click and the onSubmit mock in the updated
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2c69fac-1e7b-4d00-b470-721c4ba1c718
📒 Files selected for processing (5)
.changeset/red-hats-jam.mdpackages/form-core/src/FieldApi.tspackages/form-core/src/FormApi.tspackages/form-core/tests/FieldApi.spec.tspackages/react-form/tests/useField.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1364-1375: The teardown skips aborting async work started by the
older FieldApi instance because it only avoids touching shared controllers when
a newer instance is mounted; fix by either tracking abort controllers per
FieldApi instance or by ignoring async completions when the instance no longer
matches. Concretely: change the validation logic so each FieldApi stores its own
controllers (e.g., a per-instance map on this) instead of reusing
fieldInfo.validationMetaMap.lastAbortController, and abort only those on
teardown; or add a guard in the async validator completion path (before calling
field.setMeta(...) / inside FieldApi's async result handling) that checks
field.getInfo().instance === this and returns early if it differs. Ensure you
reference validationMetaMap, lastAbortController, FieldApi, setMeta and getInfo
when locating the relevant code to update.
- Around line 1330-1359: Currently async validation/listener
timeouts/controllers are attached to the active instance (this) even when the
async work targets a different field, so the target field cannot cancel them on
unmount; update the code that schedules linked-field async work (the
validateAsync caller that writes to getInfo().validationMetaMap and
this.timeoutIds.validations / this.timeoutIds.listeners) to store ownership on
the target field instead of this (i.e., use the target field's timeoutIds and
validationMetaMap entry keys), and update the teardown returned by the FieldApi
cleanup to clear any timeouts/controllers stored on the field itself (iterate
and clear entries on field.timeoutIds.validations/listeners/formListeners and
remove related validationMetaMap entries) so the target field’s unmount cancels
its own async work. Ensure you reference and update validateAsync,
getInfo().validationMetaMap, and timeoutIds.validations/listeners/formListeners
usage sites so ownership is moved to the field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39798198-5e48-4350-8160-450fd6a0e1ea
📒 Files selected for processing (3)
packages/form-core/src/FieldApi.tspackages/form-core/tests/FieldApi.spec.tspackages/react-form/tests/useField.test.tsx
Add unmounting to fields.
field.mountnow returns a cleanup function for unmounting.Without unmounting, conditionally rendered fields will keep their field-level validations running even though the field isn’t rendered anymore.
Summary by CodeRabbit
New Features
Bug Fixes
Tests